Addressing transform sort order with null values #4783
Conversation
oshikryu
force-pushed
the
sort-order-with-null
branch
from
f997283 to
9a4d3ca
Compare
src/transforms/sort.js
Outdated
| } else if(b.v === null) { | ||
| return 1; | ||
| } | ||
| return d2c(a.v) - d2c(b.v); |
There was a problem hiding this comment.
Thanks for the PR @oshikryu - looking good, and your test additions are excellent!
I think we want to treat any sort of invalid data the same as null - which, after running through d2c, becomes BADNUM from src/constants/numerical. BADNUM is just undefined but for clarity and consistency we prefer to use the named constant.
So this would look something like:
var ac = d2c(a.v);
var bc = d2c(b.v);
if(ac === BADNUM) {
return -1;
}
if(bc === BADNUM) {
return 1;
}
return ac - bc;Also I suppose this can be up for discussion, but in several other contexts (eg Dash DataTable) we sort null values always at the end - the idea being you want them out of the way where you mostly ignore them. Here you're sorting them as "extra negative", which puts them last when sorting descending, but FIRST when sorting ascending.
There was a problem hiding this comment.
Good point about "extra negative" for BADNUM values. I added the reference to BADNUM in the sort and updated the tests
Changing sort transform tests to include check for BADNUM Sort ascending has BADNUM last (not "extra negative" where BADNUM is first) Sort descending has BADNUM last
oshikryu
force-pushed
the
sort-order-with-null
branch
from
9a4d3ca to
d267337
Compare
|
I'm not really sure what downstream errors were produced with this change. I could use some guidance @alexcjohnson |
There was a problem hiding this comment.
Excellent, thanks for the change, this looks good to me! 💃 @archmoj please merge when you're ready.
I'm not really sure what downstream errors were produced with this change.
Were the tests failing before your force push? Don't worry about it, the tests have been a little bit flaky lately and sometimes need to be rerun. For future reference though we discourage force pushes in this repo once a PR is open, as it can confuse the review process.
Updating sort function to handle null value case
Changing sort transform tests to include null value
This should fix the issue in #4784
Example working codepen:
https://codepen.io/oshikryu/pen/rNOjeZb